[SoftDeleteable] Remove dollar sign from cache key #2978

adambalint-srg · 2025-08-22T10:06:15Z

This PR removes dollar sign ($) from the default cache key. It's not a globally reserved characters in filenames, but there are cases when it can cause problems, as I read. This is why e.g. Laminas's filesystem cache disables this character. I think using __ instead of _$ isn't a problem, and won't cause any problems.

VincentLanglet · 2025-09-14T21:34:56Z

Hi, do you have any example when it's an issue ?

adambalint-srg · 2025-09-15T05:51:51Z

In my application (Laminas+Doctrine stack) I'm using this repo's extensions, and for Doctrine I'm using Laminas's filesystem cache. So the cacheId extension metadata will be used in file system cache. Now it throws an error. I've created a quick example where you can reproduce:

{
    "name": "test/test",
    "require": {
		"php": "^8.3",
		"laminas/laminas-cache-storage-adapter-filesystem": "^3.1.0",
		"gedmo/doctrine-extensions": "^3.20.1"
	}
}

<?php

declare(strict_types=1);

include 'vendor/autoload.php';

use Gedmo\Mapping\ExtensionMetadataFactory;
use Laminas\Cache\Storage\Adapter\Filesystem;

$cacheId = ExtensionMetadataFactory::getCacheId('Foo', 'Bar');
//cacheId is 'Foo_$BAR_CLASSMETADATA';

$fileSystemCache = new Filesystem();
$fileSystemCache->addItem($cacheId, ['some' => 'data']);
//Uncaught Laminas\Cache\Exception\InvalidArgumentException: The key 'Foo_$BAR_CLASSMETADATA' doesn't match against pattern '/^[a-z0-9_\+\-\.]*$/Di'

mbabker · 2025-09-15T13:00:10Z

Since this library requires a PSR-6 cache, and from what I can tell the Laminas cache doesn't directly implement that PSR (it does provide a decorator for this), is there an issue when you use their PSR-6 decorator with this library? (On a side note, I'd say the ExtensionMetadataFactory::getCacheId() really should've been marked internal and not used externally or in the exact way that reproducer snippet is written, but it's a little late for that now)

From a practical perspective, there isn't an issue with this PR (and presumably improves PSR-6 compatibility by ensuring only characters the spec explicitly says an implementation must support are used). But, it seems that the MongoDB ODM also uses a dollar sign in its cache keys which would imply that ODM and Laminas Cache are incompatible for the same reason.

adambalint-srg · 2025-09-16T06:16:43Z

@mbabker
I'm using PSR-6 decorator in my application. But this decorator does nothing with the cache keys.

/** @var StorageAdapterFactoryInterface $storageFactory */
		$storageFactory = $container->get(StorageAdapterFactoryInterface::class);

		/** @var StorageInterface&FlushableInterface $fileCache */
		$fileCache = $storageFactory->create('filesystem', $options);

		return new CacheItemPoolDecorator($fileCache);

The direct use of the getCacheId is only for this example, I don't use it directly.

phansys

Thank you @adambalint-srg!

As you are presenting this change as a fix, please add an entry in the Fixed section of the changelog, including an explanation about what it fixes.

See: https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/CONTRIBUTING.md#changelog.

Thanks also to @mbabker for the detailed analysis.

# Conflicts: # CHANGELOG.md

adambalint-srg · 2025-09-22T10:01:32Z

@phansys Changelog has been updated

CHANGELOG.md

VincentLanglet · 2025-09-22T17:54:11Z

I think a new rebase is needed since 3.21 is released

Bumps [hadolint/hadolint-action](https://github.com/hadolint/hadolint-action) from 3.2.0 to 3.3.0. - [Release notes](https://github.com/hadolint/hadolint-action/releases) - [Changelog](https://github.com/hadolint/hadolint-action/blob/master/.releaserc) - [Commits](hadolint/hadolint-action@v3.2.0...v3.3.0) --- updated-dependencies: - dependency-name: hadolint/hadolint-action dependency-version: 3.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

VincentLanglet · 2025-09-23T07:52:59Z

CHANGELOG.md

+### Fixed
+- SoftDeleteable: Remove dollar sign ($) from the default cache key of soft deleteable metadata - to be compatible with stricter cache key naming conventions (#2978)


This should be the only change in the CHANGELOG.md file, you shouldn't impact 3.21.0 release

CHANGELOG.md

Co-authored-by: Vincent Langlet <[email protected]>

adambalint-srg · 2025-09-23T08:04:19Z

@VincentLanglet Sorry for mixing changelog parts. Is it OK now?

VincentLanglet · 2025-09-23T08:12:35Z

@VincentLanglet Sorry for mixing changelog parts. Is it OK now?

I think it is. It's on @phansys side now

codecov · 2025-09-23T09:51:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.56%. Comparing base (eb53dfc) to head (9f12943).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2978   +/-   ##
=======================================
  Coverage   78.56%   78.56%           
=======================================
  Files         169      169           
  Lines        8803     8803           
=======================================
  Hits         6916     6916           
  Misses       1887     1887

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

phansys · 2025-09-23T10:08:28Z

CHANGELOG.md


 ## [Unreleased]
+### Fixed
+- SoftDeleteable: Remove dollar sign ($) from the default cache key of soft deleteable metadata - to be compatible with stricter cache key naming conventions (see https://www.php-fig.org/psr/psr-6/#definitions) (#2978)


Reading the provided description and the referenced definition, I can't see this change as a fix.
The PSR-6 spec about cache keys states the following:

Key - A string of at least one character that uniquely identifies a cached item. Implementing libraries MUST support keys consisting of the characters A-Z, a-z, 0-9, _, and . in any order in UTF-8 encoding and a length of up to 64 characters. Implementing libraries MAY support additional characters and encodings or longer lengths, but must support at least that minimum.

This does not mean that characters other than A-Z, a-z, 0-9, _, and . are not allowed or discouraged. It confirms that at least these must be supported as minimum.

However, even if the current choice of characters in this library complies with the PSR-6 spec but causes a problem, the Fixed section MUST explicitly and clearly state what problem this change is fixing.

@phansys
Should I link the Laminas File cache's pattern here?
https://github.com/laminas/laminas-cache-storage-adapter-filesystem/blob/3.2.x/src/FilesystemOptions.php#L26
This was the reason of the whole PR.

In my opinion if PSR6 says about what is the most strict standard, and this library follows this, it can be a fix, but it's your decision of course.

Here's my take on the changelog entry. It's not a fix per se as it's not directly fixing a bug, so let's call it a change. Also note it doesn't affect only SoftDeleteable as this is changing the base EMF class all extensions use.

### Changed - All: Removed the dollar sign from the generated cache ID for extension metadata to ensure only characters mandated by PSR-6 are used, improving compatibility with caching implementations with strict character requirements (#2978)

FWIW, doctrine/persistence originally used $CLASSMETADATA as its cache salt in the class metadata factory when the doctrine/cache library was used. It changed to the current __CLASSMETADATA__ value in doctrine/persistence#144 when support for PSR-6 was added and support for the Doctrine Cache library was deprecated. There isn't any explanation in that PR why the salt changed.

I agree with your proposal @mbabker.

@adambalint-srg, could you please rebase the PR and update the changelog according this suggestion?

adambalint-srg added 3 commits October 4, 2021 08:23

Handle array parameters in QueryAnalyzer

2aa6ce4

Merge branch 'doctrine-extensions:main' into main

d1df20e

refactor: Remove dollar sign from cache key

b9f4b8f

mbabker approved these changes Sep 16, 2025

View reviewed changes

phansys added Bug A confirmed bug in Extensions that needs fixing. Needs Changelog note Needs Rebase labels Sep 20, 2025

phansys reviewed Sep 20, 2025

View reviewed changes

adambalint-srg changed the title ~~refactor: Remove dollar sign from cache key~~ [SoftDeleteable] Remove dollar sign from cache key Sep 22, 2025

adambalint-srg added 5 commits September 22, 2025 11:53

Cache key dollar sign - Add changelog entry

0683d13

Merge remote-tracking branch 'upstream/main'

0ed5521

Merge branch 'main' into cache-key-fix

f8bc8a8

# Conflicts: # CHANGELOG.md

fix: Remove not related change

fdae25a

fix: Remove not related change

78de563

phansys reviewed Sep 22, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

phansys removed the Needs Changelog note label Sep 22, 2025

dependabot bot and others added 4 commits September 23, 2025 09:50

Add a deprecations baseline to ignore selected deprecations in tests

5f63ac6

3.21.0

7f32378

Cache key dollar sign - Add changelog entry

6226afd

VincentLanglet reviewed Sep 23, 2025

View reviewed changes

adambalint-srg added 2 commits September 23, 2025 09:53

Merge branch 'main' into cache-key-fix

3fceb08

docs: Add psr6 url to changelog

1acc141

VincentLanglet reviewed Sep 23, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

dd26d78

Co-authored-by: Vincent Langlet <[email protected]>

VincentLanglet approved these changes Sep 23, 2025

View reviewed changes

phansys reviewed Sep 23, 2025

View reviewed changes

phansys removed the Bug A confirmed bug in Extensions that needs fixing. label Sep 23, 2025

docs: Changelog update (All extensions are affected)

9f12943

		### Fixed
		- SoftDeleteable: Remove dollar sign ($) from the default cache key of soft deleteable metadata - to be compatible with stricter cache key naming conventions (#2978)

Uh oh!

[SoftDeleteable] Remove dollar sign from cache key #2978

Are you sure you want to change the base?

[SoftDeleteable] Remove dollar sign from cache key #2978

Conversation

adambalint-srg commented Aug 22, 2025

Uh oh!

VincentLanglet commented Sep 14, 2025

Uh oh!

adambalint-srg commented Sep 15, 2025

Uh oh!

mbabker commented Sep 15, 2025

Uh oh!

adambalint-srg commented Sep 16, 2025

Uh oh!

phansys left a comment

Choose a reason for hiding this comment

Uh oh!

adambalint-srg commented Sep 22, 2025

Uh oh!

Uh oh!

VincentLanglet commented Sep 22, 2025

Uh oh!

VincentLanglet Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adambalint-srg commented Sep 23, 2025

Uh oh!

VincentLanglet commented Sep 23, 2025

Uh oh!

codecov bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

phansys Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambalint-srg Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

mbabker Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

phansys Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 23, 2025 •

edited

Loading

phansys Sep 23, 2025 •

edited

Loading